fix(gc): a Symbol's description no longer lives in an untraced payload slot (#7246) - #7697
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughFresh symbol descriptions are copied off the GC heap into ID-keyed storage. Readers use a shared lookup helper. GC pruning removes descriptions for dead symbols. Runtime-root tests validate pointer clearing, survival, and cleanup. ChangesSymbol description lifetime fix
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant SymbolAllocation
participant FreshDescriptionStorage
participant GarbageCollector
participant SymbolAPI
SymbolAllocation->>FreshDescriptionStorage: copy description bytes by symbol ID
SymbolAllocation->>GarbageCollector: allocate symbol with null description pointer
GarbageCollector->>FreshDescriptionStorage: prune dead symbol IDs
SymbolAPI->>FreshDescriptionStorage: retrieve description bytes
FreshDescriptionStorage-->>SymbolAPI: return description text
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Re-verified after the WTF-8 refinement ( The interned description became Rebuilt
|
…d slot (#7246) `SymbolHeader::description` was a `*mut StringHeader` inside a payload the collector treats as opaque bytes. `alloc_symbol` gc_malloc's the header as `GC_TYPE_STRING`, whose type info is `pointer_free: true` / `GcRewriteDescriptorKind::Leaf` / `GcLayoutSlotKind::None` — correct for a string, whose payload IS bytes, and wrong for a symbol, whose payload's third word was a heap pointer. Symbols and strings share one GC type, so no descriptor could distinguish them. A symbol that was itself perfectly rooted could have its description reaped or relocated out from under it, and `String(sym)` / `sym.description` then read recycled memory. `SYMBOL_POINTERS` did not close it: `scan_symbol_pointer_metadata_roots_mut` uses `visit_metadata_usize_slot`, which rewrites a recorded address WITHOUT marking, and never looks at `(*ptr).description` at all. The pointer is REMOVED rather than traced. `alloc_symbol` copies the description text off the GC heap before it allocates and leaves the field null; `FRESH_SYMBOL_DESCRIPTIONS` holds it, keyed on `SymbolHeader::id`. Why that beat the other two candidates the issue listed: * the key is the ID, which an evacuation copies verbatim — so the table needs no rekey pass, no root scanner and no budgeted step twin (where #7239 found the one real drift). It holds no GC pointer at all. * the text is copied BEFORE the allocation, so there is no window in which a description pointer is live-but-untraced. #7341's `RuntimeHandleScope` + `across_mut` in `alloc_symbol` is gone with it: it made the STORED pointer correct across `gc_malloc`, and there is no longer a stored pointer. * a `GC_TYPE_SYMBOL` would have been the principled fix but touches 190 `GC_TYPE_STRING` sites across runtime and codegen, plus the type table's verification contract. * the descriptions are pruned in `prune_dead_symbol_pointers` on the same liveness verdict that prunes `SYMBOL_POINTERS`, which pays down the retention cost the issue named as this option's price. `alloc_symbol` has exactly two callers, both fresh (`Symbol()` / `Symbol(desc)`); registered and well-known symbols are `Box::leak`'d and keep using the process-global `REGISTERED_SYMBOL_DESCRIPTIONS`. Ids are globally monotonic, so the thread-local and process-global maps never collide. The four readers now go through one `symbol_description_text` helper instead of open-coding the `registered_symbol_description(..).or_else(..)` chain. Witness, the issue's own reproducer, same compiler, A/B across the runtime rebuild (`PERRY_GC_HEAP_LIMIT=8 PERRY_GC_INCREMENTAL=0 PERRY_CONSERVATIVE_STACK_SCAN=off PERRY_GC_FORCE_EVACUATE=1`): before: B 1 5/5 deterministic after: B 0 10/10 (node 26.5.1: B 0) Movement confirmed live on the after-run: 87 copying minors, `copied_objects` 6008 / 4743 on the two that mattered. Plus three knob-free unit tests in `gc/tests/runtime_roots/symbol_description.rs` — structural (the payload pointer stays null), behavioural (the description survives reclamation of the string it came from, with the from-space bytes recycled into 'Z'-filled strings first so a stale read cannot pass by luck), and the prune. Sabotage-verified: restoring `(*ptr).description = description` fails all three.
…7246) `str_from_header` UTF-8-validates and returns `None` on failure, and a description built from a JS string carrying a lone surrogate is WTF-8, not UTF-8. Interning through `String` would therefore have turned a lone-surrogate `sym.description` from a string into `undefined` — a behaviour change smuggled in on a GC fix, in an area CLAUDE.md already lists as a known gap. The interned description is now `Arc<[u8]>` and round-trips through `js_string_from_bytes` unchanged. `js_symbol_to_string` still renders lossily, which is what `str_from_header(..).unwrap_or_default()` did before: a WTF-8 description was never formattable into a Rust `String` losslessly. Residual, stated in the code rather than hidden: the rebuilt `StringHeader` does not carry `STRING_FLAG_HAS_LONE_SURROGATES`, because the original flag is not recoverable from the payload. Pre-existing WTF-8 gap, and strictly better than dropping the description.
a29187a to
4ca7719
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/perry-runtime/src/gc/tests/runtime_roots/symbol_description.rs (1)
118-162: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for a lone-surrogate description.
The PR chose
Arc<[u8]>overArc<str>specifically so a WTF-8 description is not turned intoundefinedby UTF-8 validation. No test locks that behaviour in. A future change back toArc<str>or tostr_from_headerwould pass all three tests here.Add a fourth test that allocates a symbol whose description contains a lone surrogate, then asserts
js_symbol_descriptionreturns a string rather thanundefinedand that the bytes round-trip unchanged.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry-runtime/src/gc/tests/runtime_roots/symbol_description.rs` around lines 118 - 162, In the runtime roots symbol-description tests, add a dedicated test alongside dead_symbols_descriptions_are_pruned_with_their_pointers that allocates a symbol from a lone-surrogate WTF-8 description, calls js_symbol_description, and asserts the result is a string rather than undefined with bytes identical to the original description. Reuse the existing GC guards, symbol allocation, string-byte inspection, and cleanup helpers so the test verifies unchanged round-tripping.crates/perry-runtime/src/symbol.rs (1)
453-467: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winKeep the description id with each registered symbol pointer.
prune_dead_symbol_pointerskeeps entries for foreign process-global symbol pointers becausegc::dead_ownerskips unattributable owners, then dereferences them here. Store the id in the scan/prune state or at registration time so this pass never reads(*ptr).idfor retained pointers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry-runtime/src/symbol.rs` around lines 453 - 467, Update prune_dead_symbol_pointers and the SYMBOL_POINTERS registration state to retain each symbol pointer together with its description id, captured when the symbol is registered or scanned. Use the stored id when populating live_ids after retain, eliminating the unsafe dereference of retained pointers while preserving dead-pointer pruning.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/perry-runtime/src/gc/tests/runtime_roots/symbol_description.rs`:
- Around line 118-162: In the runtime roots symbol-description tests, add a
dedicated test alongside
dead_symbols_descriptions_are_pruned_with_their_pointers that allocates a symbol
from a lone-surrogate WTF-8 description, calls js_symbol_description, and
asserts the result is a string rather than undefined with bytes identical to the
original description. Reuse the existing GC guards, symbol allocation,
string-byte inspection, and cleanup helpers so the test verifies unchanged
round-tripping.
In `@crates/perry-runtime/src/symbol.rs`:
- Around line 453-467: Update prune_dead_symbol_pointers and the SYMBOL_POINTERS
registration state to retain each symbol pointer together with its description
id, captured when the symbol is registered or scanned. Use the stored id when
populating live_ids after retain, eliminating the unsafe dereference of retained
pointers while preserving dead-pointer pruning.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 962a424b-c784-48a4-bcad-9b6b9ae7b1c4
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
CLAUDE.mdCargo.tomlchangelog.d/7697-symbol-description-offheap.mdcrates/perry-runtime/src/gc/tests/runtime_roots.rscrates/perry-runtime/src/gc/tests/runtime_roots/symbol_description.rscrates/perry-runtime/src/symbol.rscrates/perry-runtime/src/symbol/constructors.rscrates/perry-runtime/src/symbol/properties.rs
addr_class_inventory ratcheted symbol.rs at 3 handle-floor sites; the two new dereference guards added a 4th and 5th. A bare < 0x1000 floor does not reject the fetch/zlib/proxy handle bands, which segfault on Linux. Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix
Audit — merging as v0.5.1400, with one fix appliedThe defect is the sharpest kind: a type that is right for one thing and wrong for another sharing it. And Removing the pointer rather than tracing it is the right of the three candidates, and the key is why it is cheap: Verified independently, 200 symbols across 200k allocations of churn, compiled with
One fix I applied
Worth noting the gate caught this before merge because I now run all 24 through a script that exits non-zero and gate the merge on it — I merged twice today with a red gate by printing the failure and merging anyway. Gates: 24/24, |
1 participant
Closes #7246.
The defect
SymbolHeader::descriptionwas a*mut StringHeaderliving inside a payload thecollector treats as opaque bytes.
alloc_symbolgc_malloc's the header asGC_TYPE_STRING, whose type info ispointer_free: true/GcRewriteDescriptorKind::Leaf/GcLayoutSlotKind::None— correct for astring, whose payload really is bytes, and wrong for a symbol, whose
payload's third word was a heap pointer. Symbols and strings share one GC type,
so no descriptor could tell them apart.
So a symbol that was itself perfectly rooted — shadow slot, side table,
everything — could have its description reaped or relocated out from under it.
SYMBOL_POINTERSdid not close it either:scan_symbol_pointer_metadata_roots_mutvisits the set withvisit_metadata_usize_slot, which rewrites a recorded address withoutmarking, and never looks at
(*ptr).descriptionat all.The decision
The issue listed three candidates. This takes the third — intern off-heap — and
the reason it is cheap is the key.
FRESH_SYMBOL_DESCRIPTIONSis keyed onSymbolHeader::id, a monotonicu64that an evacuation copies verbatim. Not on the symbol's address. Therefore:
so there is nothing for a scanner to visit and nothing for a
(FULL, STEP)pair to drift on (fix(gc): root the ten unrooted runtime-side caches, and the scanner that walked 1 of 3 sibling slots (#7231) #7239 found the one real drift in exactly that shape);
alloc_symbolcopies the text before it allocates, so adescription pointer is never live-but-untraced. gc(layer 3): from-space quarantine catches 55 stale dereferences across the gap suite — the instrument is in CI but aimed at one synthetic fixture #7341's
RuntimeHandleScope+across_muthere goes with it — that made the stored pointer correct acrossgc_malloc, and there is no longer a stored pointer.Why not the other two:
GC_TYPE_SYMBOLis the principled fix, and it touches 190GC_TYPE_STRINGsites acrossperry-runtimeandperry-codegen, plus the GCtype table's verification contract,
is_symbol_pointer,heap_snapshot.rsand
dead_owner.rs. Worth doing one day; not worth doing hurriedly.conditional on the symbol being live, which is a weak-table ordering problem —
and
SYMBOL_POINTERScan hold an entry for a symbol that is dead but not yetpruned, so the scanner would dereference freed memory to reach the
description.
The retention cost the issue named as this option's price is paid down:
prune_dead_symbol_pointersprunes the descriptions on the same livenessverdict that prunes the pointers.
Blast radius is small, and here is why
alloc_symbolhas exactly two callers, both fresh symbols(
js_symbol_new_empty,js_symbol_new). Registered (Symbol.for) andwell-known symbols are
Box::leak'd and already used the process-globalREGISTERED_SYMBOL_DESCRIPTIONS— untouched. Ids are globally monotonic, so thethread-local and process-global maps never collide. The four readers
(
js_symbol_key_for,js_symbol_description,js_symbol_to_string,infer_symbol_function_name) now go through onesymbol_description_texthelper instead of open-coding the
registered_symbol_description(..).or_else(..)chain — there were four of them, and a fifth that forgot the fallback is exactly
how a description goes silently missing.
Witness — the issue's own reproducer, A/B across the runtime rebuild
Same compiler,
PERRY_RUNTIME_DIRpinned,PERRY_NO_AUTO_OPTIMIZE=1, nocompile-time GC env. Run arm as the issue specifies:
Movement confirmed live on the after-run (#7255): 87 copying minors, with
copied_objects=6008andcopied_objects=4743on the two that mattered. A runthat moved nothing would prove nothing. The shipped default is also
B 0.Note the count moved from the
B 2the issue recorded toB 1— same defect,one probe now failing rather than two, which is what you would expect from the
allocation-shape churn since. It is still 5/5 deterministic, which is the tell
for this class: an unrooted cache goes bad at collection #0 and stays bad.
Unit tests — knob-free, and each one able to fail
crates/perry-runtime/src/gc/tests/runtime_roots/symbol_description.rs:SymbolHeader::descriptionmust stay null. This is theassertion a future change trips first, whether or not a behavioural test
happens to catch it that day.
string it came from. The recycling loop afterwards (512
'Z'-filled strings)is load-bearing: without it a stale read can find the old bytes intact and
the test passes for the wrong reason. It also asserts the payload pointer was
null all along, so it cannot silently measure the old representation.
Otherwise interning off-heap trades a use-after-free for an unbounded leak,
and the doc comment claiming otherwise would be the only evidence.
Sabotage: restoring
(*ptr).description = descriptionfails all three.Not done here
test-files/test_gap_gc_symbol_local_rooting.tsstill ships itsdescriptionless-
Symbol()carve-out and the header comment explaining it. Theissue says that can be dropped once this lands; it is a corpus-registered gap
file, so changing it wants a parity run rather than a code review, and it is
left as a follow-up.
Gates run locally
cargo fmt --all -- --check,cargo test -p perry-runtime --lib --no-fail-fast(1938 passed / 0 failed),
check_file_size.sh,check_test_registration.py,global_sink_isolation.py— all green.Summary by CodeRabbit
Bug Fixes
Documentation
Chores